[Integration][Octopus] Updated integration to ingest data from all spaces #958

oiadebayo · 2024-08-28T09:15:57Z

Description

What - An update to octopus integration to enable it ingest data from all spaces

Why - The previous version ingest all spaces but not data from all spaces, api assumes default space when SpaceId is not provided as a path parameter.

How -

Updated client to loop over the spaces to add spaceId parameter to the path of all resources
Updated webhook handling to includespaceId in the path to get single resources.
Cached the spaces to optimize performance during the resync event.
Updated the delete event to use specific Id to prevent deleting related resources
Made the example url in description of serverUrl in the spec.yaml more expressive

Type of change

Please leave one option from the following and delete the rest:

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
New Integration (non-breaking change which adds a new integration)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Non-breaking change (fix of existing functionality that will not change current behavior)
Documentation (added/updated documentation)

All tests should be run against the port production environment(using a testing org).

Core testing checklist

Integration able to create all default resources from scratch
Resync finishes successfully
Resync able to create entities
Resync able to update entities
Resync able to detect and delete entities
Scheduled resync able to abort existing resync and start a new one
Tested with at least 2 integrations from scratch
Tested with Kafka and Polling event listeners

Integration testing checklist

Integration able to create all default resources from scratch
Resync able to create entities
Resync able to update entities
Resync able to detect and delete entities
Resync finishes successfully
If new resource kind is added or updated in the integration, add example raw data, mapping and expected result to the examples folder in the integration directory.
If resource kind is updated, run the integration with the example data and check if the expected result is achieved
If new resource kind is added or updated, validate that live-events for that resource are working as expected
Docs PR link here

Preflight checklist

Handled rate limiting
Handled pagination
Implemented the code in async
Support Multi account

Screenshots

Include screenshots from your environment showing how the resources of the integration will look.

API Documentation

Provide links to the API documentation used for this integration.
Octopus Rest Api docs

Tankilevitch · 2024-08-28T09:58:34Z

integrations/octopus/client.py

+    async def get_paginated_resources(
+        self,


Tankilevitch · 2024-08-28T09:59:34Z

integrations/octopus/client.py

    async def get_single_resource(
-        self, resource_kind: str, resource_id: str
+        self, resource_kind: str, resource_id: str, space_id: str


what happens if you send it an empty string? as you do when you get event from a real time webhook

It will return 404, but the code has been slightly modified so it will always have a space_id when making this call. All events has the space_id present in the payload

PeyGis

requested some changes

PeyGis · 2024-08-28T09:38:05Z

integrations/octopus/CHANGELOG.md

+
+### Improvements
+
+- Updated integration to ingest all spaces


Updated the integration to ingest resources from all spaces instead of the default space

PeyGis · 2024-08-28T09:44:14Z

integrations/octopus/client.py

        params: Optional[dict[str, Any]] = None,
+        kind: Optional[str] = None,


you have an optional kind parameter but I don't see where you are using it in this _get_paginated_function. Also may I know why you decided to create a private helper method for pagination instead of using the existing one?

The kind is used in line 76 and it is no longer optional. I have joined the code for the paginator method together

PeyGis · 2024-08-28T09:45:56Z

integrations/octopus/client.py

+        self,
+        kind: str,
+        params: Optional[dict[str, Any]] = None,
+        path_parameter: Optional[str] = None,


what is the use of this path_parameter?

The space id goes in the path not the query parameter (params), it is a path parameter

PeyGis · 2024-08-28T09:52:51Z

integrations/octopus/client.py

        """Get all spaces in the Octopus instance."""
-        return await self._send_api_request("spaces/all")
+        async for spaces in self.get_paginated_resources("space"):


Let's use the Objectkind.SPACE instead

PeyGis · 2024-08-28T09:55:51Z

integrations/octopus/main.py

@@ -77,21 +109,27 @@ async def handle_webhook_request(data: Dict[str, Any]) -> Dict[str, Any]:
    payload = data.get("Payload", {}).get("Event", {})
    related_document_ids = payload.get("RelatedDocumentIds", [])
    event_category = payload.get("Category", "")
+    space_id = payload.get("SpaceId", "")


Under what circumstance will a webhook event not have the SpaceId? If we don't get the space then we just return since we don't know which space this event is coming from. if you are sure that we will get spaceId then use

space_id = payload["SpaceId"]

Updated to space_id = payload["SpaceId"] it is always present

PeyGis · 2024-08-28T09:59:30Z

integrations/octopus/main.py

+        resource_id = (
+            payload.get("ChangeDetails", {}).get("DocumentContext", {}).get("Id")
+        )
+        if resource_id.split("-")[0].lower() in TRACKED_EVENTS:


resource_id may not exist so let's be defensive and check if it exist before splitting

PeyGis · 2024-08-28T10:08:30Z

integrations/octopus/main.py

+    async for spaces in octopus_client.get_all_spaces():
+        for space in spaces:
+            async for resource_batch in octopus_client.get_paginated_resources(
+                kind, path_parameter=space.get("Id")
+            ):
+                logger.info(f"Received batch of {len(resource_batch)} {kind} ")
+                yield resource_batch
+


Since we have nested pagination, we can use this helper function

from port_ocean.utils.async_iterators import stream_async_iterators_tasks

tasks = [ get paginated resource (kind, space.get("Id")) for space in spaces ] resource_data = [] async for batch in stream_async_iterators_tasks(*tasks): yield batch

Done, thank you for the insight

Tankilevitch · 2024-08-29T14:56:59Z

integrations/octopus/client.py

+            logger.error(f"Unexpected error for space '{space_id}': {str(e)}")
        return {"ok": True}


you don't need to return anything iiuc

I have removed

PeyGis

left some more sugestions

PeyGis · 2024-08-30T16:25:22Z

integrations/octopus/main.py

+                existing_webhook_names = [
+                    subscription.get("Name", "") for subscription in subscriptions
+                ]


is there a reason we are checking for webhook name instead of using the URL?

We need to track both, unlike other webhooks I have worked with, name is required and must be unique when creating octopus subscription and the url is not required since the subscription can be an email subscription too. Tracking url let us know if that particular url has been previously created and tracking the name if the url does not exist only helps avoid making an api call that will definitely throw a 400

This situation will only happen in rare cases whereby a customer decides to switch hosts, like maybe point a different subdomain at the integration

PeyGis · 2024-08-30T16:26:40Z

integrations/octopus/main.py

+    async for spaces in octopus_client.get_all_spaces():
+        for space in spaces:
+            async for subscriptions in octopus_client.get_webhook_subscriptions(
+                space.get("Id")


you may need to check if space.get("Id") is not None, so that it doesn't cause errors.

PeyGis · 2024-08-30T16:29:49Z

integrations/octopus/main.py

+                elif any(
+                    name.startswith("Port Subscription -")
+                    for name in existing_webhook_names
+                ):
+                    logger.info(
+                        "Webhook already exists with name starting with 'Port Subscription -'"
+                    )


if webhook names checking isn't important then let's remove this

PeyGis · 2024-08-30T16:31:49Z

integrations/octopus/main.py

+    async for spaces in octopus_client.get_all_spaces():
+        for space in spaces:
+            async for subscriptions in octopus_client.get_webhook_subscriptions(
+                space.get("Id")


I see 3 nested loops here. This will make it slow during the application startup process, especially when they have huge spaces. Is it possible to run some in parallel or take advantage of the stream iterator helper function you used in the global resync?

PeyGis

some minor changes

PeyGis · 2024-09-03T14:27:21Z

integrations/octopus/main.py

+    async for spaces in octopus_client.get_all_spaces():
+        for space in spaces:
+            async for subscriptions in octopus_client.get_webhook_subscriptions(
+                space.get("Id")


can we be defensive and catch possible None type of space.get('iD') before passing it to get_webhook_subscriptions? I want us to avoid scenario where we pass None to the API

PeyGis · 2024-09-03T14:30:22Z

integrations/octopus/main.py

+                    )
+                else:
+                    await octopus_client.create_webhook_subscription(
+                        app_host, space.get("Id")


Since space can be None here, can we try to avoid sending API calls with None values?

PeyGis

LGTM

integrations/octopus/main.py

omby8888

Nice work 🌊

Update-octopus-integration-to-ingest-all-spaces

b8c1da4

oiadebayo requested a review from PeyGis August 28, 2024 09:15

Merge branch 'main' into Update-octopus-integration-to-ingest-all-spaces

1c07a9f

github-actions bot added the size/M label Aug 28, 2024

Tankilevitch reviewed Aug 28, 2024

View reviewed changes

integrations/octopus/client.py Outdated

Comment on lines 74 to 75

async def get_paginated_resources(

self,

Copy link

Contributor

Tankilevitch Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

Tankilevitch reviewed Aug 28, 2024

View reviewed changes

PeyGis requested changes Aug 28, 2024

View reviewed changes

PeyGis and others added 3 commits August 28, 2024 10:17

Merge branch 'main' into Update-octopus-integration-to-ingest-all-spaces

f206873

Merge branch 'main' into Update-octopus-integration-to-ingest-all-spaces

693473f

Attended to revision comments

646f959

Tankilevitch reviewed Aug 29, 2024

View reviewed changes

oiadebayo and others added 2 commits August 29, 2024 17:27

Removed unnecessary return

21f5dce

Merge branch 'main' into Update-octopus-integration-to-ingest-all-spaces

cf8a03b

PeyGis requested changes Aug 30, 2024

View reviewed changes

PeyGis requested changes Sep 3, 2024

View reviewed changes

oiadebayo and others added 6 commits September 3, 2024 23:37

Merge branch 'main' into Update-octopus-integration-to-ingest-all-spaces

e63929e

Merge branch 'main' into Update-octopus-integration-to-ingest-all-spaces

be01392

Attended to reviewer's suggestion

9f4317b

Merge branch 'main' into Update-octopus-integration-to-ingest-all-spaces

bdcad60

Update pyproject.toml

632b026

Merge branch 'main' into Update-octopus-integration-to-ingest-all-spaces

be29169

PeyGis approved these changes Sep 11, 2024

View reviewed changes

integrations/octopus/main.py Outdated Show resolved Hide resolved

oiadebayo and others added 8 commits September 12, 2024 00:28

Update main.py

0d6f0ce

Merge branch 'main' into Update-octopus-integration-to-ingest-all-spaces

db72954

Merge branch 'main' into Update-octopus-integration-to-ingest-all-spaces

1b6c722

Update pyproject.toml

1768226

Merge branch 'main' into Update-octopus-integration-to-ingest-all-spaces

34d5377

Merge branch 'main' into Update-octopus-integration-to-ingest-all-spaces

4e3f7e9

Merge branch 'main' into Update-octopus-integration-to-ingest-all-spaces

4fb5b32

Merge branch 'main' into Update-octopus-integration-to-ingest-all-spaces

2a64298

Updated changelog

81d19a5

omby8888 approved these changes Oct 10, 2024

View reviewed changes

PeyGis merged commit 9fe159b into main Oct 10, 2024
16 checks passed

PeyGis deleted the Update-octopus-integration-to-ingest-all-spaces branch October 10, 2024 13:44

		params: Optional[dict[str, Any]] = None,
		kind: Optional[str] = None,

		logger.error(f"Unexpected error for space '{space_id}': {str(e)}")
		return {"ok": True}

[Integration][Octopus] Updated integration to ingest data from all spaces #958

[Integration][Octopus] Updated integration to ingest data from all spaces #958

Conversation

oiadebayo commented Aug 28, 2024

Description

Type of change

All tests should be run against the port production environment(using a testing org).

Core testing checklist

Integration testing checklist

Preflight checklist

Screenshots

API Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeyGis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oiadebayo Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeyGis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeyGis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeyGis left a comment

Choose a reason for hiding this comment

omby8888 left a comment

Choose a reason for hiding this comment

oiadebayo Aug 28, 2024 •

edited

Loading